-
Notifications
You must be signed in to change notification settings - Fork 152
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Moved cache queue before of the store get function. (up to 2x performance boost). #18
Conversation
Thanks! Can you please run "make" and fix the lint and test errors? |
Thanks, I'll check this out in more detail ASAP. |
Great thanks! |
I've added domain support to make sure the wrap callback function is always called. |
Sorry, I got busy and didn't get around to checking this out in more detail. I will today. |
delete self.queues[key]; | ||
domain | ||
.create() | ||
.on('error', function(err) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aletorrado - Do you know if there's a way to get test coverage for this domain error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can just throw an exception in the work function and check that the
callback function is called only once and that it receives that error.
El dic 18, 2014 8:19 PM, "Bryan Donovan" notifications@github.com
escribió:
In lib/caching.js
#18 (diff)
:} else {
- self.queues[key] = [cb];
work(function () {
var work_args = Array.prototype.slice.call(arguments, 0);
if (work_args[0]) { // assume first arg is an error
self.queues[key].forEach(function (done) {
done.call(null, work_args[0]);
});
delete self.queues[key];
domain
.create()
.on('error', function(err) {
@aletorrado https://github.com/aletorrado - Do you know if there's a
way to get test coverage for this domain error?—
Reply to this email directly or view it on GitHub
https://github.com/BryanDonovan/node-cache-manager/pull/18/files#r22079610
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Can you please add tests for this? E.g.,
context("when an error is thrown in the work function", function () {
var fake_error;
beforeEach(function() {
fake_error = new Error(support.random.string());
});
it("bubbles up that error", function (done) {
cache.wrap(key, function () {
throw fake_error;
}, ttl, function (err) {
assert.equal(err, fake_error);
done();
});
});
});
around line 360 in caching.unit.js and something similar in multi_caching.unit.js?
Thanks, I'll merge now. |
Moved cache queue before of the store get function. (up to 2x performance boost).
This is in 0.15.0 now. |
Great! Thank you!
|
Hi! This change prevents the cache store from being bursted. I'm getting up to 200% performance boost when using a local redis store, and should be higher with caches located farther.